-
Notifications
You must be signed in to change notification settings - Fork 730
telemetry: add project account id and region #8079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
8c03419 to
d427165
Compare
|
No issues |
liuzulin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test detail in PR description seems indicating we only tested when domain account id === project account id. Can you test with an actual x region x account set up. I can send you the project info
| * @returns Promise resolving to the account ID | ||
| * @throws ToolkitError if unable to extract account ID | ||
| */ | ||
| private async extractAccountIdFromResourceMetadata(): Promise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to util file. for example smusUtils.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is moved to the util file.
| return this.cachedProjectAccountIds.get(projectId)! | ||
| } | ||
|
|
||
| // If in SMUS space environment, extract account ID from resource-metadata file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the one in resource-metadata is always domain account id? can you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resourceArn in resource-metadata.json gives the project id.
| logger.debug('Fetching project account ID via STS GetCallerIdentity with project credentials') | ||
|
|
||
| // Get project credentials | ||
| const projectCredProvider = await this.getProjectCredentialProvider(projectId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can me move this as a util function as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may leave it there because, first the logic is not used in other places, second it follows getDomainAccountId theme.
| let projectRegion: string | undefined | ||
|
|
||
| if (projectId) { | ||
| projectAccountId = await authProvider.getProjectAccountId(projectId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to confirm, this method is only used by retrieving project account id for right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
2506e00 to
30e58e8
Compare
30e58e8 to
7bb0309
Compare
Problem
The project account id and region was missing from data connection and space action telemetry.
Solution
The project account id and region are added to data connection and space action telemetry.
Telemetry for a cross-account example:
feature/xbranches will not be squash-merged at release time.